Skip to content

Conversation

@JAJames
Copy link

@JAJames JAJames commented Mar 4, 2025

Fixes #91: Memory corruption on connecting to Online Lobby
This should remain a draft PR until this has been tested.

Analysis:

  • bind_ptr->name.ids is currently being assigned to some stack memory, instead of the copy that's allocated via SnmpUtilMemAlloc. That's probably the main cause of the originally reported issue
  • SnmpExtensionQuery is documented as potentially allocating/reallocating/freeing memory on the VarBindList. This is validated by the fact that the reported crash happens in that function, when it attempts to free the aforementioned stack memory. So for safety, we should probably assume it invalidates any pointers.
  • SnmpUtilVarBindListFree handles cleaning up members of the SnmpVarBindList. Based on example usage and peeking at the ReactOS implementation, it seems like the actual SnmpVarBindList is not free'd by the function, because it's not necessarily expected to be allocated via SnmpUtilMemAlloc. So there's an opportunity to clean this up to not require calling SnmpUtilMemFree directly at all.
  • The existing code doesn't free some of the members of the binds, so there's possibly a small memory leak here
  • If we assign mib_ii_name_ptr directly to name.ids without cleaning up some of these other issues, we risk a double-free and small memory leak

Changes:

  • Changed bind_ptr->name.ids assignment to actually use the SnmpUtilMemAlloc allocated memory, rather than the static array sitting in stack, to satisfy requirements indicated by MSDN documentation
  • Cleaned up the memory handling around this in general, so that we can just call SnmpUtilVarBindListFree and be done.

Testing:

  • None at all; I don't have a great reproduction case, and I haven't actually tried compiling locally yet, as I'm waiting on the CMake branch to be merged.

This can probably be merged after some basic testing.

@xezon
Copy link

xezon commented Mar 4, 2025

This is amazing. I am so happy to see that we will crush these bugs like flies.

@xezon xezon added Bug Something is not working right, typically is user facing Critical Severity: Minor < Major < Critical < Blocker labels Mar 4, 2025
@DevGeniusCode DevGeniusCode added the ZH Relates to Zero Hour label Mar 4, 2025
@xezon xezon changed the title #91: Fix invalid memory access, potential leak Fix invalid memory access when connecting to Online Lobby Mar 30, 2025
@xezon xezon modified the milestones: Linux support, Stability fixes Apr 28, 2025
@Mauller
Copy link

Mauller commented Jul 15, 2025

This want's rebasing with main

@xezon
Copy link

xezon commented Sep 15, 2025

I think we still dont have code in place to connect to online lobby? If not, then we need that to test this change. Otherwise needs some localhost tinkering

@Skyaero42
Copy link

I think we still dont have code in place to connect to online lobby? If not, then we need that to test this change. Otherwise needs some localhost tinkering

Will we ever? With Generals Online being the new facto online-experience I don't think there is much use in maintaining GameSpy functionality. Unless it is really needed for Revora (?). But I feel like Revora is dying fast (currently 24 online players with 2 lobbies, GO is atm 318 online players).

We haven't seen JAJames around for a while, so maybe better to just close this draft PR.

@Mauller
Copy link

Mauller commented Nov 6, 2025

Yes, Generals online does not use any of the gamespy related code

@tintinhamans
Copy link

Related #516

@Skyaero42
Copy link

Because of the aforementioned reasons, I'm closing this PR.

If the author wants to continue with this PR, they can reopen it.

@Skyaero42 Skyaero42 closed this Nov 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is not working right, typically is user facing Critical Severity: Minor < Major < Critical < Blocker ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory corruption on connecting to Online Lobby

6 participants